Skip to content

Enforce Java 11 response body timeouts - #3462

Open
goutamadwant wants to merge 2 commits into
OpenFeign:masterfrom
goutamadwant:fix-http2-response-timeout
Open

Enforce Java 11 response body timeouts#3462
goutamadwant wants to merge 2 commits into
OpenFeign:masterfrom
goutamadwant:fix-http2-response-timeout

Conversation

@goutamadwant

Copy link
Copy Markdown

Summary

This updates the Java 11 Http2Client response handling so the configured Feign read timeout also applies while the response body is being read.

The existing request timeout still covers waiting for the response headers, but BodyHandlers.ofInputStream() can hand back a response before the body has arrived. The response body stream is now wrapped with a read-timeout guard, preserving the existing streaming response behavior while failing stalled body reads with HttpTimeoutException.

Tests

  • mvn --batch-mode -Dtoolchain.skip=true -pl java11 -am -Dtest=Http2ClientTest#timeoutReadingResponseBody,Http2ClientAsyncTest#timeoutReadingResponseBody -Dsurefire.failIfNoSpecifiedTests=false test
  • mvn --batch-mode -Dtoolchain.skip=true -pl java11 -am -Dtest=Http2ClientTest,Http2ClientAsyncTest -Dsurefire.failIfNoSpecifiedTests=false test
  • mvn --batch-mode -Dtoolchain.skip=true -pl java11 -am -Dsurefire.failIfNoSpecifiedTests=false test
  • mvn --batch-mode -Dtoolchain.skip=true -pl java11 -am -DskipTests validate
  • mvn --batch-mode -Dtoolchain.skip=true -pl java11 -am git-code-format:validate-code-format

Fixes #3068

@velo

velo commented Jul 28, 2026

Copy link
Copy Markdown
Member

This feels like it belong on a interceptor or capability.... it is an orthogonal feature that we could apply to all clients.

@velo velo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good problem to solve — the Java 11 client genuinely ignores readTimeout once headers arrive, and I like that you covered both the sync and async paths with tests. Two things need fixing before this can go in.

1. Subclass overrides of toFeignResponse are silently bypassed

execute() now calls the new private three-arg toFeignResponse(Request, HttpResponse, Options). The existing protected Response toFeignResponse(Request, HttpResponse) is left in place but is no longer on the execution path — so any subclass overriding it (a documented extension point today) keeps compiling and keeps passing its own tests, while its code stops being called in production. That's the worst kind of breaking change: invisible.

Http2ClientContentLengthTest hides this, because it calls the two-arg form directly and therefore exercises a path production no longer uses.

Please make the three-arg overload protected and reduce the two-arg one to a delegating shim (return toFeignResponse(request, httpResponse, null);), so overriding either signature still works.

2. One scheduled task per read() call

readWithTimeout schedules a ScheduledFuture and cancels it on every invocation, including the single-byte read(). For a consumer reading unbuffered, that's a schedule+cancel round-trip on a shared single-threaded executor per byte. Two allocations of AtomicBoolean per byte on top.

Schedule one deadline for the stream (or at least per read(byte[], int, int) batch) and check remaining time, rather than arming a fresh timer per read.

Smaller notes

  • TimeoutInputStream doesn't delegate skip, mark, reset, markSupported. InputStream#skip routes through read, so it's correct, but markSupported silently degrades to false for a delegate that supported it.
  • BODY_READ_TIMEOUT_EXECUTOR is a process-wide static that's never shut down. Daemon thread makes that survivable, but it means every Http2Client in the JVM shares one timer thread — worth a comment at minimum.
  • In readWithTimeout, the throw timeoutException == null ? timeoutException(e) : timeoutException; line reads awkwardly. if (timeoutException != null) throw timeoutException; throw timeoutException(e); is clearer.
  • Please make sure the new tests use Retryer.NEVER_RETRY consistently — the async one doesn't set it, so a retry would multiply the 1s body delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Http2Client with native HttpClient does not enforce response timeout correctly

2 participants